-
-
Notifications
You must be signed in to change notification settings - Fork 970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add HardwareIntrinsics AVX-512 info #2412
Conversation
We will need to update the sdk to net8 before the CI can run the tests, which I think will have to wait until net8 is officially released. |
if (IsX86Avx512FSupported) yield return "AVX-512F"; | ||
if (IsX86Avx512BWSupported) yield return "AVX-512BW"; | ||
if (IsX86Avx512CDSupported) yield return "AVX-512CD"; | ||
if (IsX86Avx512DQSupported) yield return "AVX-512DQ"; | ||
if (IsX86Avx512VbmiSupported) yield return "AVX-512VBMI"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other main path is an if/else if
pattern.
This new code will yield/return several AVX-512F ISAs and not in their reverse hierarchical order. -- It is also missing the AVX512VL
ISA, which is nested under the respective classes (its one flag shared across all, but dependent on the containing class also being supported).
Is that intentional?
I'd imagine we want to cover Avx512F
in the main if/else chain and the others to be on top to be "inline" with how the other works.
Notably certain combinations also have well-defined names and it may be "better" to list the well-known name to avoid a list that is 20 ISAs long.
That is, x86-64-v4
is the formal definition that includes AVX512F, AVX512BW, AVX512CD, AVX512DQ, AVX512VL
x86-64-v3
is AVX, AVX2, BMI1, BMI2, F16C, FMA, LZCNT, MOVBE, OSXSAVE
x86-64-v2
is CMPXCHG16B, POPCNT, SSE3, SSE4.1, SSE4.2, SSSE3
x86-64-v1
(baseline) is CMOV, CX8, x87FPU, FXSR, MMX, OSFXSR, SCE, SSE, SSE2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably argue against using x86-x64-v?
since it not immediately obvious what's what.
Instead perhaps we could list it as AVX-512(X/Y/Z)
e.g. AVX-512(F/BW/CD/DQ/VL)
. This makes set obvious and immediately visible.
Can confirm that it works.
Notably, I think it would be beneficial if this support was added earlier (and done so every release; that includes support for any new ISAs added in .NET 9, etc). The runtime repo (and various other repos) are all using BDN to validate performance of preview versions of .NET. BDN itself adding in some preview support for the latest version of .NET, including ISAs like this, makes the entire .NET preview experience that much better. It's also notably not a lot of code and can itself be considered "preview" until the point that version of .NET does ship as stable. |
BDN already supports .Net 8 preview. This would be for our internal CI. I'll defer to @adamsitnik in regards to that. |
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nietras thank you for your contribution!
WIP untested just to ask whether this is looks like path. Feel free to edit code directly if need be or do PR to PR.